Rust: Include parameters and patterns in the CFG #17686

hvitved · 2024-10-08T12:21:22Z

This PR adds parameters and patterns to the CFG, needed for SSA. Unlike expressions, which construct values, patterns destruct values, so they are modeled pre-order in the CFG. The first exception being identifier patterns

match value {
   x @ Option::None =>  // ..
   // ...
}

where we need to first match against Option::None and only bind x in case of a match, and the second exception being | patterns, which are modeled like logical disjunction expressions.

Examples

Simple parameters

fn mutate_param(x : &mut i64) { // x
    *x = // $ read_access=x
        *x + // $ read_access=x
        *x; // $ read_access=x
}

Before

flowchart TD
1["enter mutate_param"]
10["... + ..."]
11["x"]
12["* ..."]
13["x"]
2["exit mutate_param"]
3["exit mutate_param (normal)"]
4["BlockExpr"]
5["* ..."]
6["... = ..."]
7["ExprStmt"]
8["x"]
9["* ..."]

1 --> 7
3 --> 2
4 --> 3
5 --> 11
6 --> 4
7 --> 8
8 --> 5
9 --> 13
10 --> 6
11 --> 9
12 --> 10
13 --> 12

After

flowchart TD
1["enter mutate_param"]
10["x"]
11["* ..."]
12["... + ..."]
13["x"]
14["* ..."]
15["x"]
2["exit mutate_param"]
3["exit mutate_param (normal)"]
4["x"]
5["Param"]
6["BlockExpr"]
7["* ..."]
8["... = ..."]
9["ExprStmt"]

1 --> 4
3 --> 2
4 -- match --> 5
5 --> 9
6 --> 3
7 --> 13
8 --> 6
9 --> 10
10 --> 7
11 --> 15
12 --> 8
13 --> 11
14 --> 12
15 --> 14

Match expression

fn match_pattern5() {
    let either = Either::Left(32); // either
    match either { // $ read_access=either
        Either::Left(a3) | Either::Right(a3) // a3
            => print_i64(a3), // $ read_access=a3
    }
}

Before

flowchart TD
1["enter match_pattern5"]
10["MatchExpr"]
11["either"]
12["OrPat"]
13["PathExpr"]
14["CallExpr"]
15["a3"]
2["exit match_pattern5"]
3["exit match_pattern5 (normal)"]
4["BlockExpr"]
5["LetStmt"]
6["either"]
7["PathExpr"]
8["CallExpr"]
9["32"]

1 --> 5
3 --> 2
4 --> 3
5 --> 7
6 -- match, no-match --> 11
7 --> 9
8 --> 6
9 --> 8
10 --> 4
11 --> 12
12 -- match --> 13
13 --> 15
14 --> 10
15 --> 14

After

flowchart TD
1["enter match_pattern5"]
10["MatchExpr"]
11["either"]
12["TupleStructPat"]
13["[match(true)] OrPat"]
14["a3"]
15["TupleStructPat"]
16["a3"]
17["PathExpr"]
18["CallExpr"]
19["a3"]
2["exit match_pattern5"]
3["exit match_pattern5 (normal)"]
4["BlockExpr"]
5["LetStmt"]
6["either"]
7["PathExpr"]
8["CallExpr"]
9["32"]

1 --> 5
3 --> 2
4 --> 3
5 --> 7
6 -- match --> 11
7 --> 9
8 --> 6
9 --> 8
10 --> 4
11 --> 12
12 -- match --> 14
12 -- no-match --> 15
13 -- match --> 17
14 -- match --> 13
15 -- match --> 16
16 -- match --> 13
17 --> 19
18 --> 10
19 --> 18

Parameter pattern matching

fn param_pattern2(
    (Either::Left(a9) | Either::Right(a9)): Either // a9
) -> () {
    print_i64(a9); // $ read_access=a9
}

Before

flowchart TD
1["enter param_pattern2"]
2["exit param_pattern2"]
3["exit param_pattern2 (normal)"]
4["BlockExpr"]
5["PathExpr"]
6["CallExpr"]
7["ExprStmt"]
8["a9"]

1 --> 7
3 --> 2
4 --> 3
5 --> 8
6 --> 4
7 --> 5
8 --> 6

After

flowchart TD
1["enter param_pattern2"]
10["BlockExpr"]
11["PathExpr"]
12["CallExpr"]
13["ExprStmt"]
14["a9"]
2["exit param_pattern2"]
3["exit param_pattern2 (normal)"]
4["Param"]
5["TupleStructPat"]
6["[match(true)] OrPat"]
7["a9"]
8["TupleStructPat"]
9["a9"]

1 --> 5
3 --> 2
4 --> 13
5 -- match --> 7
5 -- no-match --> 8
6 -- match --> 4
7 -- match --> 6
8 -- match --> 9
9 -- match --> 6
10 --> 3
11 --> 14
12 --> 10
13 --> 11
14 --> 12

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

rust/ql/lib/codeql/rust/controlflow/internal/Splitting.qll

aibaars · 2024-10-11T11:53:57Z

rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll

@@ -117,13 +117,61 @@ class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
  override string toString() { result = "boolean(" + value + ")" }
 }

+/** Holds if node `pat` has the constant match value `value`. */


I think Rust use the term irrefutable pattern for a pattern that cannot fail.

I don't think this is correct use of "irrefutable". Irrefutable is property of a pattern, but this predicate is a property of a pattern as an AST node and considers the surrounding AST context as well.

For instance, in

match b { Some(a) => 1, None => 0 }

this predicate holds for None (since the last pattern in a match arm is guaranteed to succeed due to exhaustiveness) but None is not an irrefutable pattern.

I think we should rename this predicate (not sure to what though). We could also extract a proper isIrrefutablePattern predicate from this predicate (but it will probably be a conservative approximation as refutability for custom types depends on the type definition. E.g. the pattern Foo(a) is irrefutable if Foo the only case in the enum and refutable otherwise).

Yeah, you're right. Perhaps isExhaustiveMatch or similar. An irrefutable pattern is exhaustive on its own, and a last pattern in match should be exhaustive.

That sounds like a great name to me. I'll do a PR :)

Done, PR is here #17751

aibaars · 2024-10-11T11:55:45Z

rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll

+/** Holds if node `pat` has the constant match value `value`. */
+pragma[nomagic]
+private predicate isMatchConstant(Pat pat, boolean value) {
+  value = true and


Perhaps remove the value and it seems to be always true

aibaars · 2024-10-11T11:57:37Z

rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll

+    // parameter patterns must be exhaustive
+    pat = any(Param p).getPat()
+  ) and
+  not pat = any(ForExpr for).getPat() // workaround until `for` loops are desugared


Not sure I understand why you exclude patterns in for here, they should also never fail

Thats because of how we currently model for loops; the pattern is used to represent the condition.

aibaars · 2024-10-11T12:01:39Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

@@ -141,11 +138,10 @@ class LogicalAndTree extends PostOrderTree, LogicalAndExpr {

 class BlockExprTree extends StandardPostOrderTree, BlockExpr {
  override AstNode getChildNode(int i) {
-    result = super.getStmtList().getStatement(i)


Why this change? I see inconsistent use of instanceof SomeClass and extends ..., SomeClass in this file. Wouldn't it be better to consistently use instanceof ?

Better to consistently use extends.

aibaars · 2024-10-11T12:10:17Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

+// Closures have their own CFG scope, so we need to make sure that their
+// CFG is not mixed with the surrounding CFG. This is done by retrofitting
+// `first`, `propagatesAbnormal`, and `succ` below.
+class ClosureExprTree extends StandardPostOrderTree, ClosureExpr {


I'm not sure I like the way this is formulated. If I understand correctly you want the succ relation of a StandardOrderTree and the first and last predicates of a LeafTree. Perhaps it would be clearer to make this class extend StandardOrderTree and avoid the not succ = this restriction in succ and add an explicit last predicate. It's about the same amount of lines and you avoid the tweaks to "undo" the behaviour of a Post order tree.

Ah, yes, I had completely forgotten about StandardTree.

aibaars · 2024-10-11T12:13:40Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

+ * Provides `ControlFlowTree`s for patterns.
+ *
+ * Since patterns destruct values, they are modeled in pre-order, except for
+ * `OrPat`s and `IdentPat`s.


Why are ident and or-patterns different?

Because in e @ pat we only "evalute" e when pat matches. And in pat1 | pat2 we need it to be post-order in order to get the correct dominance information needed for guards.

aibaars · 2024-10-11T12:15:35Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

+
+  class LiteralPatTree extends LeafTree, LiteralPat { }
+
+  class MacroPatTree extends LeafTree, MacroPat { } // todo


Done, in another PR ;-)

aibaars · 2024-10-11T12:17:49Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

+    override Pat getPat(int i) { result = this.getField(i) }
+  }
+
+  class ConstBlockPatTree extends LeafTree, ConstBlockPat { } // todo?


I think the constant value inside is evaluated compile-time, so perhaps having this as a LeafTree is the right thing to do.

github-actions bot added C# Ruby Rust Pull requests that update Rust code Swift labels Oct 8, 2024

github-advanced-security bot found potential problems Oct 8, 2024

View reviewed changes

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll Fixed Show fixed Hide fixed

rust/ql/lib/codeql/rust/controlflow/internal/Splitting.qll Fixed Show fixed Hide fixed

hvitved force-pushed the rust/cfg-patterns branch 9 times, most recently from da0bcd6 to 9ed1394 Compare October 10, 2024 11:22

github-actions bot removed C# Ruby Swift labels Oct 10, 2024

hvitved marked this pull request as ready for review October 10, 2024 11:40

hvitved requested a review from paldepind October 10, 2024 12:03

hvitved added 4 commits October 10, 2024 21:04

Rust: Add CFG test for nested function

665da39

Rust: Include parameters in the CFG

f6f54c6

Rust: Include patterns in the CFG

2f14ec9

Rust: Fix bugs in LetExprTree and MethodCallExprTree

756affa

hvitved force-pushed the rust/cfg-patterns branch from 9ed1394 to 756affa Compare October 10, 2024 19:07

aibaars reviewed Oct 11, 2024

View reviewed changes

Address review comments

ffd6b98

aibaars approved these changes Oct 11, 2024

View reviewed changes

hvitved merged commit ac8b973 into github:main Oct 11, 2024
14 checks passed

hvitved deleted the rust/cfg-patterns branch October 11, 2024 12:45

paldepind mentioned this pull request Oct 14, 2024

Rust: Rename isIrrefutablePattern to isExhaustiveMatch #17751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Include parameters and patterns in the CFG #17686

Rust: Include parameters and patterns in the CFG #17686

hvitved commented Oct 8, 2024 •

edited

Loading

aibaars Oct 11, 2024

paldepind Oct 14, 2024

aibaars Oct 14, 2024

paldepind Oct 14, 2024

paldepind Oct 14, 2024

aibaars Oct 11, 2024

aibaars Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 11, 2024

aibaars Oct 11, 2024


		class LiteralPatTree extends LeafTree, LiteralPat { }

		class MacroPatTree extends LeafTree, MacroPat { } // todo

Rust: Include parameters and patterns in the CFG #17686

Rust: Include parameters and patterns in the CFG #17686

Conversation

hvitved commented Oct 8, 2024 • edited Loading

Examples

Simple parameters

Match expression

Parameter pattern matching

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved commented Oct 8, 2024 •

edited

Loading